Skip to content

T6 (#813): Remove client main() + repoint per-jar integrity to lib/MANIFEST.MF#821

Open
GOKULRAJ136 wants to merge 3 commits into
mosip:developfrom
GOKULRAJ136:T6-813-RC
Open

T6 (#813): Remove client main() + repoint per-jar integrity to lib/MANIFEST.MF#821
GOKULRAJ136 wants to merge 3 commits into
mosip:developfrom
GOKULRAJ136:T6-813-RC

Conversation

@GOKULRAJ136

@GOKULRAJ136 GOKULRAJ136 commented Jun 29, 2026

Copy link
Copy Markdown

Refer Story #807
Fixes #813

Summary by CodeRabbit

  • Bug Fixes
    • Improved update/package validation to require and verify both root and lib/ manifest files in the correct locations.
    • Client integrity verification now fails closed when lib/MANIFEST.MF is missing or cannot be loaded.
    • Update cleanup no longer removes the lib/MANIFEST.MF file or its detached signature.
  • Refactor
    • Removed the legacy standalone client entry point and simplified startup manifest/plugin configuration.
  • Tests
    • Updated validators’ tests to set up and clean lib/MANIFEST.MF deterministically, including fail-closed coverage.

…ib/MANIFEST.MF

Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@GOKULRAJ136, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 91b9daec-a5c0-4f40-a723-5f77ca2a6b2b

📥 Commits

Reviewing files that changed from the base of the PR and between 260e006 and 168b39e.

📒 Files selected for processing (1)
  • registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientIntegrityValidatorTest.java

Walkthrough

The PR moves per-jar integrity data to lib/MANIFEST.MF, updates validation to use libManifest, prevents the manifest files from being deleted, aligns tests with the new layout, and removes the Initialization entry point and related references.

Changes

lib/MANIFEST.MF integrity split and Initialization removal

Layer / File(s) Summary
ClientIntegrityValidator: lib manifest path and fail-closed enforcement
registration/registration-services/src/main/java/io/mosip/registration/update/ClientIntegrityValidator.java
Redirects the manifest path to lib/MANIFEST.MF, throws SecurityException when the manifest is absent, and refactors getLocalManifest to check file existence and load the manifest with try-with-resources.
ClientSetupValidator: libManifest loading and validation
registration/registration-services/src/main/java/io/mosip/registration/update/ClientSetupValidator.java
Adds libManifestFile and libManifest, initializes the new manifest in the constructor, requires it in validateBuildSetup(), switches jar deletion and checksum iteration to libManifest, and factors manifest loading through loadManifest.
SoftwareUpdateUtil: protect manifest files from deletion
registration/registration-services/src/main/java/io/mosip/registration/update/SoftwareUpdateUtil.java
Adds constants for MANIFEST.MF and MANIFEST.MF.sig, and updates deleteUnknownJars to skip those files under lib/.
Tests: align integrity and setup checks with lib manifest
registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientIntegrityValidatorTest.java, registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientSetupValidatorTest.java, registration/registration-services/src/test/java/io/mosip/registration/test/update/ManifestCreatorTest.java
Updates tests to manage lib/MANIFEST.MF, adds fail-closed and manifest-present cases, resets and assigns libManifest, and copies the generated manifest into lib/.
Remove Initialization entry point and references
registration/registration-client/pom.xml, registration/registration-client/src/main/java/io/mosip/registration/controller/Initialization.java, registration/registration-client/src/main/java/io/mosip/registration/util/control/...
Removes mainClass from the JAR and JavaFX Maven plugin configs, deletes the Initialization class, and drops its unused imports from the control classes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • anushasunkada
  • Prafulrakhade
  • JanardhanBS-SyncByte

Poem

🐰 Two manifests now, one root and one in lib,
The rabbit hops where hashes live and nibble-giggle glib.
No stray jars vanish in the night,
The entry class has left the site.
Hop hop—integrity shines bright!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.52% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the two main changes: removing the client main entry point and moving per-jar integrity checks to lib/MANIFEST.MF.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@registration/registration-services/src/main/java/io/mosip/registration/update/ClientIntegrityValidator.java`:
- Around line 57-64: The fallback branch in ClientIntegrityValidator should not
treat opening a JarFile with verification enabled as sufficient when
trustedCertificate is missing. Update the logic around verifyIntegrity and the
else path so the client integrity check explicitly reads/verifies signed JarFile
entries (or otherwise fails fast) instead of logging success after constructing
JarFile only. Make sure the missing provider.pem/provider.cer case fails closed
and does not bypass integrity enforcement.

In
`@registration/registration-services/src/main/java/io/mosip/registration/update/ClientSetupValidator.java`:
- Around line 102-109: The same-version validation path is using a stale
constructor-loaded libManifest, so jar checksum checks can miss updates after
the manifest rewrite. Refresh or reload libManifest from the updated jar state
before iterating its entries in ClientSetupValidator, and make sure the checksum
validation logic uses the current per-jar manifest data rather than the original
local copy.
- Around line 160-162: The IOException catch in ClientSetupValidator currently
hides the underlying manifest load failure by throwing a RegBaseCheckedException
without the cause. Update the exception handling in the manifest-loading path to
use the RegBaseCheckedException constructor that accepts the caught IOException,
so the failure reason is preserved for cases like permission or corruption
issues; keep the existing errorCode and label message, but pass e through when
rethrowing.

In
`@registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientIntegrityValidatorTest.java`:
- Around line 55-80: The `ClientIntegrityValidatorTest` methods are sharing the
fixed working-directory `lib/MANIFEST.MF` path, which makes them flaky when test
order changes or when another test like `ManifestCreatorTest` also touches the
same file. Update `verifyClientIntegrityTest` and
`verifyClientIntegrityManifestPresentTest` to save the pre-existing
`lib/MANIFEST.MF` and `lib/` state before modifying them, then restore or clean
up that state in teardown/finally so the tests remain isolated and do not depend
on external filesystem state.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9d0019ce-11d6-4fc1-befc-9887aaffaebf

📥 Commits

Reviewing files that changed from the base of the PR and between 3954574 and 8e3611b.

📒 Files selected for processing (13)
  • registration/registration-client/pom.xml
  • registration/registration-client/src/main/java/io/mosip/registration/controller/Initialization.java
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/FxControl.java
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/impl/ButtonFxControl.java
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/impl/CheckBoxFxControl.java
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/impl/DOBFxControl.java
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/impl/DropDownFxControl.java
  • registration/registration-services/src/main/java/io/mosip/registration/update/ClientIntegrityValidator.java
  • registration/registration-services/src/main/java/io/mosip/registration/update/ClientSetupValidator.java
  • registration/registration-services/src/main/java/io/mosip/registration/update/SoftwareUpdateUtil.java
  • registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientIntegrityValidatorTest.java
  • registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientSetupValidatorTest.java
  • registration/registration-services/src/test/java/io/mosip/registration/test/update/ManifestCreatorTest.java
💤 Files with no reviewable changes (7)
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/impl/CheckBoxFxControl.java
  • registration/registration-client/src/main/java/io/mosip/registration/controller/Initialization.java
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/impl/DropDownFxControl.java
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/impl/ButtonFxControl.java
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/impl/DOBFxControl.java
  • registration/registration-client/pom.xml
  • registration/registration-client/src/main/java/io/mosip/registration/util/control/FxControl.java

Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientIntegrityValidatorTest.java (1)

81-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the missing-manifest failure, not just SecurityException.
verifyClientIntegrity() wraps many failures in the same exception type, so this test can go green for the wrong reason. Check the nested cause/message for lib/MANIFEST.MF to exercise the intended path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientIntegrityValidatorTest.java`
around lines 81 - 84, The verifyClientIntegrityTest currently only asserts
SecurityException, which can pass for unrelated failures; update the test around
ClientIntegrityValidator.verifyClientIntegrity() to verify the missing-manifest
path specifically by asserting the nested cause or message mentions
lib/MANIFEST.MF, so the test exercises the intended failure instead of any
generic security error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientIntegrityValidatorTest.java`:
- Around line 60-75: In ClientIntegrityValidatorTest, the setup and teardown
logic around lib/MANIFEST.MF ignores failures from delete() and renameTo(),
which can leave the manifest in an unexpected state. Update the backup/restore
flow in the init/restore methods to fail fast by checking each file operation
result or using checked file move/delete APIs, so test state is reliably
restored via libManifest, libManifestBackup, and restoreLibManifest.

---

Outside diff comments:
In
`@registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientIntegrityValidatorTest.java`:
- Around line 81-84: The verifyClientIntegrityTest currently only asserts
SecurityException, which can pass for unrelated failures; update the test around
ClientIntegrityValidator.verifyClientIntegrity() to verify the missing-manifest
path specifically by asserting the nested cause or message mentions
lib/MANIFEST.MF, so the test exercises the intended failure instead of any
generic security error.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6179e854-9ee8-4a76-8ee1-6252d2ecd0f2

📥 Commits

Reviewing files that changed from the base of the PR and between 8e3611b and 260e006.

📒 Files selected for processing (3)
  • registration/registration-services/src/main/java/io/mosip/registration/update/ClientIntegrityValidator.java
  • registration/registration-services/src/main/java/io/mosip/registration/update/ClientSetupValidator.java
  • registration/registration-services/src/test/java/io/mosip/registration/test/update/ClientIntegrityValidatorTest.java

Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant